search updates #14
Merged
search updates #14
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
prasmussen15
temporarily deployed
to
development
GitHub Actions
Inactive
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to f8643f1 in 58 seconds
More details
- Looked at
561lines of code in9files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. core/utils/maintenance/graph_data_operations.py:3
- Draft comment:
LiteralStringis imported but not used in this file. Consider removing it to clean up the imports. - Reason this comment was not posted:
Confidence changes required:10%
The import statement forLiteralStringis unnecessary ingraph_data_operations.pysince it's not used anywhere in the file.
2. core/search/search.py:40
- Draft comment:
Theretrieve_episodesfunction is called without specifyinglast_n, which defaults toEPISODE_WINDOW_LEN. Ensure this is the intended behavior or consider passingconfig.num_episodesexplicitly. - Reason this comment was not posted:
Confidence changes required:50%
Insearch.py, theretrieve_episodesfunction is called without passing thelast_nparameter, which defaults toEPISODE_WINDOW_LEN. This might not be the intended behavior if the user wants to specify a different number of episodes.
3. tests/graphiti_tests_int.py:71
- Draft comment:
Consider callingawait graphiti.build_indices_and_constraints()to ensure indices and constraints are set up before running the search tests. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment suggests a method that is not present in the code, which makes it speculative. The original methodbuild_indiceswas removed, and the comment suggests a replacement that is not defined in the code. This makes the comment not actionable as it stands.
The methodbuild_indices_and_constraintsmight exist elsewhere in the codebase, and the comment could be suggesting a valid improvement. However, without evidence of its existence in the provided context, the comment remains speculative.
The comment should be based on the code visible in the diff. Without evidence of the method's existence, the comment is speculative and not actionable.
The comment should be removed as it suggests a speculative change without evidence of the method's existence in the provided code.
Workflow ID: wflow_6SOrSShknXW1hrHa
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
prasmussen15
temporarily deployed
to
development
GitHub Actions
Inactive
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 4696de1 in 48 seconds
More details
- Looked at
204lines of code in4files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. core/search/search.py:34
- Draft comment:
The return type annotation should bedict[str, list[Node | Edge]]instead ofdict[str, [Node | Edge]]. - Reason this comment was not posted:
Confidence changes required:50%
The return type annotation in thesearchfunction incore/search/search.pyis incorrect. It should bedict[str, list[Node | Edge]]instead ofdict[str, [Node | Edge]]. This is a minor issue but should be corrected for clarity and correctness.
2. core/search/search_utils.py:332
- Draft comment:
Ensure float division is used for scoring calculation to avoid integer division issues. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is unnecessary because the code already uses float division. In Python, dividing an integer by another integer using the/operator results in a float. Therefore, there is no risk of integer division here.
I might be overlooking any specific context where integer division could still occur, but given the code provided, it seems unlikely.
The code clearly uses float division, and the comment does not add any value. It is safe to assume that the division will result in a float.
The comment should be removed because it is unnecessary; the code already ensures float division.
3. core/search/search_utils.py:105
- Draft comment:
Ensure that removingDISTINCTfrom the query does not lead to unintended duplicate results. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is speculative and asks the author to verify the behavior, which is against the rules. The removal of 'DISTINCT' could potentially lead to duplicate results, but the comment does not provide a clear, actionable change. It is more of a suggestion to verify behavior, which is not allowed.
I might be overlooking the potential impact of removing 'DISTINCT', but the comment does not provide a specific code change or refactor, just a suggestion to verify behavior.
Even if the removal of 'DISTINCT' could lead to issues, the comment does not provide a specific code change or refactor, just a suggestion to verify behavior, which is not allowed.
The comment should be removed as it is speculative and does not provide a clear, actionable change.
4. core/search/search_utils.py:155
- Draft comment:
Ensure that removingDISTINCTfrom the query does not lead to unintended duplicate results. - Reason this comment was not posted:
Marked as duplicate.
5. core/search/search_utils.py:189
- Draft comment:
Ensure that removingDISTINCTfrom the query does not lead to unintended duplicate results. - Reason this comment was not posted:
Marked as duplicate.
6. core/search/search_utils.py:228
- Draft comment:
Ensure that removingDISTINCTfrom the query does not lead to unintended duplicate results. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_vyfxPlnmPnKKnylH
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
prasmussen15
temporarily deployed
to
development
GitHub Actions
Inactive
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 704a032 in 35 seconds
More details
- Looked at
163lines of code in4files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_oZYem1KEfnZWHL0z
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
paul-paliychuk
approved these changes
Aug 22, 2024
prasmussen15
temporarily deployed
to
development
GitHub Actions
Inactive
There was a problem hiding this comment.
❌ Changes requested. Incremental review on a159cda in 24 seconds
More details
- Looked at
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_JVHoHon3cqZL22zf
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
|
||
| return facts | ||
|
|
||
| async def _search(self, query: str, timestamp: datetime, config: SearchConfig): |
There was a problem hiding this comment.
The _search method is defined but not used anywhere in the class. Consider removing it if it's not needed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Refactored search functionality, introduced
hybrid_search, updatedGraphiticlass, consolidated index creation, and updated tests.Key points:
core/searchdirectory.hybrid_searchfunction incore/search/search.py.Graphiticlass incore/graphiti.pyto usehybrid_search.build_indices_and_constraintsincore/utils/maintenance/graph_data_operations.py.build_indicesandsearchmethods fromGraphiticlass.core/prompts/dedupe_nodes.py.tests/tests_int_graphiti.pyto reflect changes.Generated with ❤️ by ellipsis.dev